diagnostics_channel: return original thenable#62407
Open
Qard wants to merge 1 commit intonodejs:mainfrom
Open
Conversation
Qard
added
confirmed-bug
rochdev
reviewed
Mar 23, 2026
Qard
force-pushed
the
trace-promise-return-original-thenable
branch
from
fc213c4 to
3eb07ea
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62407 +/- ##
==========================================
- Coverage 89.71% 89.68% -0.03%
==========================================
Files 692 692
Lines 213986 214005 +19
Branches 41055 41053 -2
==========================================
- Hits 191969 191940 -29
- Misses 14089 14140 +51
+ Partials 7928 7925 -3
🚀 New features to boost your workflow:
|
mhofman
reviewed
Mar 23, 2026
Renegade334
reviewed
Mar 24, 2026
Member
Renegade334
left a comment
Renegade334
left a comment
There was a problem hiding this comment.
Does this change have a specific case in mind? The A+ standard mandates .then() to return another "promise" instance (either the same object or a new object at the implementation's discretion), so any custom features of a compliant thenable should also be present on the object returned by .then()?
Member
Author
|
No, that is not the case. A thenable can return any variety of promise, not necessarily another of itself. It's most common that a thenable returns native promises from its then/catch, so you lose access to whatever methods it had when following the chain. |
Qard
requested review from
Renegade334,
bengl,
jasnell,
mcollina,
mhofman and
rochdev
and removed request for
mhofman and
rochdev
Renegade334
reviewed
Mar 26, 2026
Qard
force-pushed
the
trace-promise-return-original-thenable
branch
from
3eb07ea to
3d95103
Compare
Renegade334
approved these changes
Mar 26, 2026
Qard
force-pushed
the
trace-promise-return-original-thenable
branch
from
3d95103 to
c5fcc50
Compare
Renegade334
approved these changes
Mar 26, 2026
timfish
approved these changes
Mar 26, 2026
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
jasnell
reviewed
Mar 29, 2026
jasnell
reviewed
Mar 29, 2026
jasnell
approved these changes
Mar 29, 2026
Qard
force-pushed
the
trace-promise-return-original-thenable
branch
from
81fd7b1 to
3299411
Compare
Member
|
Nit: also having |
This makes tracePromise return the original thenable to allow custom thenable types to retain their methods rather than producing the chained result type.
Qard
force-pushed
the
trace-promise-return-original-thenable
branch
from
3299411 to
b0e835c
Compare
Flarna
approved these changes
Mar 29, 2026
Qard
added
request-ci
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
Collaborator
Collaborator
jasnell
reviewed
Mar 30, 2026
| return result.then(resolve, reject); | ||
| // isPromise() matches sub-classes, but we need to match only direct | ||
| // instances of the native Promise type to safely use PromisePrototypeThen. | ||
| if (isPromise(result) && result.constructor === Promise) { |
Member
There was a problem hiding this comment.
So there's an edge case here... not convinced we necessarily need to deal with it but just acknowledging... it's exceedingly unlikely that someone would do this...
class MyP extends Promise {
constructor() { super(() => {});
}
const myP = new MyP();
myP.constructor = Promise;
Member
There was a problem hiding this comment.
might at least be worth adding a test for
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This makes tracePromise return the original thenable to allow custom thenable types to retain their methods rather than producing the chained result type.
This branches the behaviour between native promises and thenables such that native promises retain the correct unhandledRejection behaviour while thenables produce the correct original return value and so retain methods present on that type.
I think that because native promises are consistently shaped it doesn't actually matter that it remains chained rather than branched.
cc @nodejs/diagnostics